Extract Maven repo interaction logic for JVM ecosystems reuse#14721
Extract Maven repo interaction logic for JVM ecosystems reuse#14721AbhishekBhaskar wants to merge 5 commits intomainfrom
Conversation
AbhishekBhaskar
changed the title
There was a problem hiding this comment.
Pull request overview
Refactors Maven repository interaction logic into a reusable shared client so other JVM ecosystems (e.g., SBT) can leverage the same Maven repository resolution behavior without duplicating code.
Changes:
- Introduces an abstract
SharedMavenRepositoryClientwith shared URL construction, metadata fetching/parsing, auth header handling, and caching. - Updates
Maven::Package::PackageDetailsFetcherto inherit from the new shared client and retain only Maven-specific orchestration/repository assembly. - Adds a comprehensive spec suite for the shared client behavior.
Show a summary per file
| File | Description |
|---|---|
| maven/lib/dependabot/maven/shared/shared_maven_repository_client.rb | New shared abstract base class encapsulating Maven repository interaction logic for reuse across JVM ecosystems. |
| maven/lib/dependabot/maven/package/package_details_fetcher.rb | Refactors Maven package details fetching to inherit shared repository logic and keep Maven-specific behavior. |
| maven/spec/dependabot/maven/shared/shared_maven_repository_client_spec.rb | Adds unit tests for shared repository client behavior (URL building, metadata parsing, caching, auth/forbidden tracking). |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
There was a problem hiding this comment.
Pull request overview
Extracts Maven repository interaction logic into a shared, abstract client so Maven (and other JVM ecosystems like SBT) can reuse Maven-repository URL construction, metadata fetching/parsing, auth header handling, and caching without duplicating code.
Changes:
- Added
SharedMavenRepositoryClientabstract base class underDependabot::Maven::Sharedwith shared Maven-repo HTTP and parsing logic. - Updated Maven
PackageDetailsFetcherto inherit from the shared client and keep only Maven-specific orchestration/repository assembly. - Added a dedicated spec suite for the shared client behaviors (URL construction, parsing, response handling, caching).
Show a summary per file
| File | Description |
|---|---|
maven/lib/dependabot/maven/shared/shared_maven_repository_client.rb |
Introduces shared Maven repository client with URL construction, registry calls, forbidden URL tracking, and metadata/release caching. |
maven/lib/dependabot/maven/package/package_details_fetcher.rb |
Refactors Maven package details fetcher to inherit shared client and override repository assembly + central repo selection. |
maven/spec/dependabot/maven/shared/shared_maven_repository_client_spec.rb |
Adds coverage for the extracted shared client logic (URLs, parsing, error/forbidden handling, caching). |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 2
| # Uses the Maven RepositoriesFinder's central URL to support credential-based overrides. | ||
| sig { override.returns(String) } | ||
| def central_repo_url | ||
| repository_finder.central_repo_url |
There was a problem hiding this comment.
central_repo_url can come from RepositoriesFinder#central_repo_url, which returns the raw credential URL when replaces-base is set (and that value may include a trailing /). Most repository URLs are normalized without trailing slashes (e.g., credentials_repository_details), so leaving this unnormalized can cause duplicate repo entries and can break central-repo comparisons in the shared client (e.g., central_repo_urls.include?(repository_url) in check_response / handle_registry_error). Consider normalizing here (strip trailing slashes) before returning.
| repository_finder.central_repo_url | |
| repository_finder.central_repo_url.sub(%r{/+\z}, "") |
| # Both HTTP and HTTPS variants of the central repo URL, for comparison. | ||
| sig { returns(T::Array[String]) } | ||
| def central_repo_urls | ||
| central_url_without_protocol = central_repo_url.gsub(%r{^.*://}, "") |
There was a problem hiding this comment.
central_repo_urls strips the protocol but does not normalize trailing slashes. If central_repo_url is overridden from credentials (e.g., replaces-base) and includes a trailing /, comparisons like central_repo_urls.include?(repository_url) may fail because repository URLs are typically normalized without trailing slashes, which can lead to incorrect forbidden URL tracking and error handling. Normalize the central URL here (e.g., strip trailing slashes) before building the http/https variants.
| central_url_without_protocol = central_repo_url.gsub(%r{^.*://}, "") | |
| central_url_without_protocol = central_repo_url | |
| .gsub(%r{^.*://}, "") | |
| .gsub(%r{/+$}, "") |
What are you trying to accomplish?
Extracts Maven repository interaction logic from
Maven::Package::PackageDetailsFetcherinto a new abstract base classMaven::Shared::SharedMavenRepositoryClient, enabling SBT (and other JVM ecosystems) to reuse Maven repository resolution without duplicating code.SBT resolves dependencies from Maven repositories using the same
maven-metadata.xmlformat, URL conventions, and auth patterns as Maven. Rather than duplicating ~300 lines of repository client logic in the SBT ecosystem, we extract it into a shared abstract class that both Maven and SBT can inherit from.Changes:
New file:
shared_maven_repository_client.rbDependabot::Maven::Shareddependency_parts,dependency_base_url,dependency_metadata_url,dependency_files_url), metadata fetching (XML + HTML), version extraction, response checking, forbidden URL tracking,released?checks, credential/auth header handling, and per-repository metadata cachingdependency,credentials,repositoriesdependency_parts(for non-standard naming),central_repo_url(for credential-based overrides)Modified file:
package_details_fetcher.rbSharedMavenRepositoryClient(was standalone class)-
fetch/releases— package details orchestration-
released?— delegates to parent via super-
repositories(override) — assembles credential repos + POM-declared repos-
central_repo_url(override) — delegates toRepositoriesFinder-
versions/versions_details_from_xml/versions_details_hash_from_html— version list assembly-
repository_finder/pom_repository_details/pom— Maven POM helpersAnything you want to highlight for special attention from reviewers?
dependencyattr_reader— same as Maven, no custom logic neededcredentialsattr_reader— same as Maven, no custom logic neededrepositoriesresolversinbuild.sbt+ credential repos via inheritedcredentials_repository_detailsHow will you know you've accomplished your goal?
If all the existing unit tests pass without failures.
Checklist